fix(overlays): show focus indicators only during keyboard navigation and improve focus management#31165
fix(overlays): show focus indicators only during keyboard navigation and improve focus management#31165brandyscarney wants to merge 27 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| @@ -114,10 +114,6 @@ slot[name="end"]::slotted(*) { | |||
|
|
|||
| // Item in Select Modal | |||
| // -------------------------------------------------- | |||
| :host(.in-select-modal) { | |||
There was a problem hiding this comment.
This was added to hide the focused state.
| @@ -15,11 +15,6 @@ ion-item { | |||
| --border-width: 0; | |||
| } | |||
|
|
|||
| ion-item.ion-focused::part(native)::after { | |||
There was a problem hiding this comment.
This was added to hide the focused state.
There was a problem hiding this comment.
The ionic theme wasn't added to this test so now it is generating screenshots.
There was a problem hiding this comment.
The focus indicator is no longer displayed when opening with a mouse click.
There was a problem hiding this comment.
The focus indicator is no longer displayed when opening with a mouse click.
There was a problem hiding this comment.
The focus indicator is no longer displayed when opening with a mouse click.
There was a problem hiding this comment.
The focus indicator is no longer displayed when opening with a mouse click.
| @@ -8,7 +8,7 @@ import { configs, test } from '@utils/test/playwright'; | |||
| * does not. The overlay rendering is already tested in the respective | |||
| * test files. | |||
| */ | |||
| configs({ directions: ['ltr'] }).forEach(({ title, config, screenshot }) => { | |||
| configs({ modes: ['ios', 'md', 'ionic-md'], directions: ['ltr'] }).forEach(({ title, config, screenshot }) => { | |||
There was a problem hiding this comment.
This now checks for the ionic theme and captures screenshots to avoid future focus behavior regressions.
ShaneK
left a comment
There was a problem hiding this comment.
This is looking really good! I tested the functionality and the changes here seem to work fine, but I noticed some things and just wanted a bit of clarification. Good work so far, though!
thetaPC
left a comment
There was a problem hiding this comment.
Questions:
- Multiple-select interfaces: if a checked option is not the first one, should that checked option receive focus when the overlay is opened with the keyboard, rather than the first option?
Issues:
- Multiple-select on popover: I can only Tab to the first two options.
- Multiple-select on popover: when I Tab into the second option and press Space to select it, it selects the first option instead.
- Action sheet: when I Tab to open it, I do not see the focus indicator on the wrapper the way I do with the alert. This happens both with no value and with a value selected.
Comments:
- The action sheet and alert behave differently from each other, and I find the wrapper ring surprising in general. I would have expected the focus indicator to be on the selected option when an overlay opens with a value, not on the wrapper. You mentioned the wrapper ring is the intended behavior: is that intended based on web standards, or is it inherited from the existing overlay focus-trap?
Issue number: internal
What is the current behavior?
The focus indicator is displayed on the option when opening certain interfaces by clicking on the Select component:
What is the new behavior?
Shift + Tabto go backwards in orderDoes this introduce a breaking change?
Other information